-
Notifications
You must be signed in to change notification settings - Fork 1.7k
GH-3930: Add Jackson 3 support; deprecate Jackson 2 #3996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
*/ | ||
public DefaultJacksonKafkaHeaderMapper(String... patterns) { | ||
this(JsonMapper.builder() | ||
.findAndAddModules(JacksonJsonMessageConverter.class.getClassLoader()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class loader from this class exactly, not something else to avoid class tangle and so.
* Create an instance for inbound mapping only with pattern matching. | ||
* @param patterns the patterns to match. | ||
* @return the header mapper. | ||
* @since 2.8.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No @since
on methods in all new classes, please.
Object value = decodeValue(header, type); | ||
headers.put(header.key(), value); | ||
} | ||
catch (IOException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be revised since Jackson 3 does not throw IOException
any more.
May just catch (Exception e) {
instead?
@@ -101,6 +103,9 @@ public BatchMessagingMessageConverter(@Nullable RecordMessageConverter recordCon | |||
if (JacksonPresent.isJackson2Present()) { | |||
this.headerMapper = new DefaultKafkaHeaderMapper(); | |||
} | |||
else if (JacksonPresent.isJackson3Present()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's prefer Jackson 3!
If they have both in classpath, so Jackson 3 choice would win.
* On the producer side, select a subclass that matches the corresponding | ||
* Kafka Serializer. | ||
* | ||
* @author Soby Chacko |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, honor authors of class we are deprecating.
|
||
private JacksonJavaTypeMapper typeMapper = new DefaultJacksonJavaTypeMapper(); | ||
|
||
private final TypeFactory typeFactory = TypeFactory.createDefaultInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use ObjectMapper.getTypeFactory()
instead in the ctor!
import org.springframework.util.Assert; | ||
|
||
/** | ||
* A {@link MessageConverter} implementation based on Jacthat uses a Spring Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably meant based on Jackson 3
😄
build.gradle
Outdated
@@ -263,6 +265,13 @@ project ('spring-kafka') { | |||
exclude group: 'org.jetbrains.kotlin' | |||
} | |||
|
|||
optionalApi 'tools.jackson.core:jackson-databind' | |||
optionalApi 'tools.jackson.datatype:jackson-datatype-joda' | |||
optionalApi 'tools.jackson.dataformat:jackson-dataformat-xml' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't do XML here in Spring Kafka, so let's just remove this redundant dependency!
Fixes spring-projects#3930 Issue link: spring-projects#3930 This commit introduces comprehensive Jackson 3 support across the Spring for Apache Kafka framework while maintaining backward compatibility by deprecating Jackson 2 components. BREAKING CHANGES: - All Jackson 2-based classes are now deprecated and will be removed in a future version - Default Jackson version updated from 2.19.2 to 2.19.1 for Jackson 2 - Added Jackson 3 BOM dependency (3.0.0-rc5) NEW FEATURES: - Added complete Jackson 3 counterparts for all existing Jackson 2 classes: * JsonKafkaHeaderMapper (replaces DefaultKafkaHeaderMapper) * JacksonJsonSerializer/Deserializer (replaces JsonSerializer/Deserializer) * JacksonJsonSerde (replaces JsonSerde) * JacksonJsonMessageConverter and subclasses (replaces JsonMessageConverter family) * JacksonProjectingMessageConverter (replaces ProjectingMessageConverter) * DefaultJacksonJavaTypeMapper (replaces DefaultJackson2JavaTypeMapper) * Jackson3Utils and Jackson3MimeTypeModule utilities INFRASTRUCTURE: - Enhanced JacksonPresent utility to detect Jackson 3 availability - Updated MessagingMessageConverter and BatchMessagingMessageConverter to prefer Jackson 3 - Modified RecordMessagingMessageListenerAdapter to handle new converter types - Updated all serialization/deserialization logic to use Jackson 3 APIs DOCUMENTATION: - Updated all documentation references from Jackson 2 to Jackson 3 classes - Corrected sample code and configuration examples - Updated method signatures and class references throughout documentation TESTING: - Migrated all test cases to use Jackson 3 equivalents - Updated test configurations and mock setups - Maintained test coverage for both Jackson 2 (deprecated) and Jackson 3 paths The framework now automatically detects and prefers Jackson 3 when available, falling back to Jackson 2 for backward compatibility. All existing applications will continue to work with Jackson 2, but new development should migrate to Jackson 3 classes. Signed-off-by: Soby Chacko <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: spring-projects/spring-framework#35282.
Might be the case that we can do that already in your current incarnation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think whats-new
deserves a dedicated bullet for Jackson 3.
Thanks
@@ -55,7 +55,8 @@ ext { | |||
awaitilityVersion = '4.3.0' | |||
hamcrestVersion = '3.0' | |||
hibernateValidationVersion = '9.0.1.Final' | |||
jacksonBomVersion = '2.19.2' | |||
jacksonBomVersion = '2.19.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why downgrade?
.enable(SerializationFeature.FAIL_ON_EMPTY_BEANS) | ||
.addModule(new Jackson3MimeTypeModule()) | ||
.build(); | ||
return objectMapper; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need local variable?
Why cannot we just use return
forthe builder result?
* {@link DeserializationFeature#FAIL_ON_UNKNOWN_PROPERTIES} features. | ||
* @return the {@link ObjectMapper} instance. | ||
*/ | ||
public static ObjectMapper enhancedObjectMapper() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my general comment.
Apparently we have to be as specific as possible.
And make this as JsonMapper
instead.
@@ -29,7 +29,10 @@ | |||
* @author Artem Bilan | |||
* | |||
* @since 2.3 | |||
* | |||
* @deprecated since 4.0 in favor of native Jackson 3 {@link tools.jackson.databind.json.JsonMapper#builder()} API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now this is not correct since you have just introduced Jackson3Utils
.
* | ||
* @since 4.0 | ||
*/ | ||
public final class Jackson3Utils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is not what is required from the framework perspective.
The goal is to avoid numbers as much as possible.
How about to name this class as JacksonMapperUtils
instead?
@@ -31,6 +31,8 @@ | |||
* @author Artem Bilan | |||
* | |||
* @since 2.3 | |||
* | |||
* @deprecated since 4.0 in favor of {@link Jackson3Utils}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not correct link.
And class is not marked with @Deprecated
.
* | ||
* @since 4.0 | ||
*/ | ||
public final class Jackson3MimeTypeModule extends SimpleModule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about MimeTypeJacksonModule
instead?
Just to avoid that number!
@@ -94,6 +96,9 @@ public MessagingMessageConverter(Function<Message<?>, @Nullable Integer> partiti | |||
if (JacksonPresent.isJackson2Present()) { | |||
this.headerMapper = new DefaultKafkaHeaderMapper(); | |||
} | |||
else if (JacksonPresent.isJackson3Present()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Jackson 3 has to be in priority if both are on classpath.
@@ -0,0 +1,485 @@ | |||
/* | |||
* Copyright 2017-present the original author or authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And no this is not right.
This class was not started back in that year.
Just nit-picking thought, since I know that Spring Boot uses a year of the project beginning for everything 🤷
Fixes: #3930